Skip to content

Add error handling for plan&execute agent #3845

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Jun 11, 2025
Merged

Add error handling for plan&execute agent #3845

merged 9 commits into from
Jun 11, 2025

Conversation

Hailong-am
Copy link
Contributor

@Hailong-am Hailong-am commented May 13, 2025 •
edited
Loading

Description

  1. Remove line separator from tool's response
  2. Index mapping tool support raw input for index parameter
  3. Add json extract logic for LLM output is not json format

Related Issues

#3848

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@Hailong-am
add more error handling
6f2c088 
Signed-off-by: Hailong Cui <ihailong@amazon.com>
@pyek-bot
Copy link
Contributor

Thanks for the fixes @Hailong-am ! Let me know when it is ready for review. Can you also please create an issue for the fixes involved in this PR? Thanks!

@Hailong-am
spotlessApply
3f00b45 
Signed-off-by: Hailong Cui <ihailong@amazon.com>
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env-require-approval May 14, 2025 05:38 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env-require-approval May 14, 2025 05:38 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env-require-approval May 14, 2025 05:38 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env-require-approval May 14, 2025 05:38 — with GitHub Actions Inactive
@Hailong-am Hailong-am marked this pull request as ready for review May 14, 2025 05:53
@Hailong-am
Copy link
Contributor Author

Thanks for the fixes @Hailong-am ! Let me know when it is ready for review. Can you also please create an issue for the fixes involved in this PR? Thanks!

@pyek-bot PR description updated with issue number and the what's the actual fix, please help to take a look

@codecov Codecov
Copy link

codecov bot commented May 14, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 80.28%. Comparing base (563120b) to head (608810a).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...thms/agent/MLPlanExecuteAndReflectAgentRunner.java 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #3845      +/-   ##
============================================
+ Coverage     80.26%   80.28%   +0.02%     
- Complexity     7815     7820       +5     
============================================
  Files           683      683              
  Lines         34513    34519       +6     
  Branches       3839     3840       +1     
============================================
+ Hits          27702    27714      +12     
+ Misses         5107     5099       -8     
- Partials       1704     1706       +2
Flag Coverage Δ
ml-commons 80.28% <87.50%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env-require-approval May 14, 2025 17:35 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env-require-approval May 14, 2025 17:35 — with GitHub Actions Inactive
pyek-bot
pyek-bot approved these changes May 16, 2025
View reviewed changes
Copy link
Contributor

@pyek-bot pyek-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixes! Looks good!

@dhrubo-os dhrubo-os had a problem deploying to ml-commons-cicd-env-require-approval June 3, 2025 19:55 — with GitHub Actions Failure
@dhrubo-os dhrubo-os had a problem deploying to ml-commons-cicd-env-require-approval June 3, 2025 19:55 — with GitHub Actions Failure
@dhrubo-os dhrubo-os had a problem deploying to ml-commons-cicd-env-require-approval June 3, 2025 19:55 — with GitHub Actions Failure
@dhrubo-os dhrubo-os had a problem deploying to ml-commons-cicd-env-require-approval June 3, 2025 19:55 — with GitHub Actions Failure
@dhrubo-os dhrubo-os had a problem deploying to ml-commons-cicd-env-require-approval June 5, 2025 23:54 — with GitHub Actions Failure
@dhrubo-os dhrubo-os had a problem deploying to ml-commons-cicd-env-require-approval June 5, 2025 23:54 — with GitHub Actions Failure
@dhrubo-os dhrubo-os had a problem deploying to ml-commons-cicd-env-require-approval June 5, 2025 23:54 — with GitHub Actions Failure
@dhrubo-os dhrubo-os had a problem deploying to ml-commons-cicd-env-require-approval June 5, 2025 23:54 — with GitHub Actions Failure
dhrubo-os
dhrubo-os reviewed Jun 6, 2025
View reviewed changes
.../main/java/org/opensearch/ml/engine/algorithms/agent/MLPlanExecuteAndReflectAgentRunner.java
@@ -528,6 +528,11 @@ String extractJsonFromMarkdown(String response) {
if (response.contains("```")) {
response = response.substring(0, response.lastIndexOf("```"));
}
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This method doesn't have any null check for response
  2. Is there any chance that multiple code block can exist?
  3. what about json array?
  4. what happens if there's any string literals like {"data": "{nested}"}
  5. Possible IndexOutOfBoundsException if markers aren't found
  6. why do we need second trim?
  7. Using substring multiple times creates multiple intermediate string objects
    Each operation like contains(), indexOf(), and lastIndexOf() scans through the string

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method doesn't have any null check for response

response passed to this method could not be null, otherwise it will throw NPE before call it. But it's a good practice to add null check here

Is there any chance that multiple code block can exist?
what about json array?
what happens if there's any string literals like {"data": "{nested}"}

Yes, all of them are possible. the logic here is try our best to extract the expected json from LLM response.

Possible IndexOutOfBoundsException if markers aren't found

if markers are not found, it will not perform substring

why do we need second trim?

the content extracted from json block may have spaces, so trim is needed here before check it's a valid json string.

Using substring multiple times creates multiple intermediate string objects
Each operation like contains(), indexOf(), and lastIndexOf() scans through the string

that's not a big concern. the response is not large, and gc will help to handle intermediate string objects

Hailong-am and others added 2 commits June 9, 2025 17:33
@Hailong-am
add unit test
d3a5bfb 
Signed-off-by: Hailong Cui <ihailong@amazon.com>
@dhrubo-os
Merge branch 'main' into agent_bug_fix
64b8aaa
ylwu-amzn
ylwu-amzn previously approved these changes Jun 10, 2025
View reviewed changes
@dhrubo-os
Copy link
Collaborator

@Hailong-am fix spotless please, ./gradlew spotlessApply

@Hailong-am
spotlessApply
03d4035 
Signed-off-by: Hailong Cui <ihailong@amazon.com>
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env-require-approval June 11, 2025 00:43 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env-require-approval June 11, 2025 00:43 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env-require-approval June 11, 2025 00:43 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env-require-approval June 11, 2025 00:43 — with GitHub Actions Inactive
@Hailong-am
Copy link
Contributor Author

@Hailong-am fix spotless please, ./gradlew spotlessApply

@dhrubo-os @ylwu-amzn I have applied the spotless, can you review it again.

@Hailong-am Hailong-am had a problem deploying to ml-commons-cicd-env-require-approval June 11, 2025 01:49 — with GitHub Actions Failure
@Hailong-am Hailong-am had a problem deploying to ml-commons-cicd-env-require-approval June 11, 2025 01:49 — with GitHub Actions Failure
@Hailong-am
add comments for index mapping tool
608810a 
Signed-off-by: Hailong Cui <ihailong@amazon.com>
@Hailong-am Hailong-am had a problem deploying to ml-commons-cicd-env-require-approval June 11, 2025 02:38 — with GitHub Actions Error
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env-require-approval June 11, 2025 02:38 — with GitHub Actions Inactive
@Hailong-am Hailong-am had a problem deploying to ml-commons-cicd-env-require-approval June 11, 2025 02:38 — with GitHub Actions Failure
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env-require-approval June 11, 2025 02:38 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env-require-approval June 11, 2025 06:13 — with GitHub Actions Inactive
@Hailong-am Hailong-am temporarily deployed to ml-commons-cicd-env-require-approval June 11, 2025 06:13 — with GitHub Actions Inactive
@ylwu-amzn ylwu-amzn merged commit 50bcbda into opensearch-project:main Jun 11, 2025
11 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mingshl mingshl mingshl left review comments

@dhrubo-os dhrubo-os dhrubo-os approved these changes

@b4sjoo b4sjoo b4sjoo approved these changes

@pyek-bot pyek-bot pyek-bot approved these changes

@ylwu-amzn ylwu-amzn ylwu-amzn left review comments

@jngz-es jngz-es Awaiting requested review from jngz-es jngz-es is a code owner

@model-collapse model-collapse Awaiting requested review from model-collapse model-collapse is a code owner

@rbhavna rbhavna Awaiting requested review from rbhavna rbhavna is a code owner

@zane-neo zane-neo Awaiting requested review from zane-neo zane-neo is a code owner

@Zhangxunmt Zhangxunmt Awaiting requested review from Zhangxunmt Zhangxunmt is a code owner

@austintlee austintlee Awaiting requested review from austintlee austintlee is a code owner

@HenryL27 HenryL27 Awaiting requested review from HenryL27 HenryL27 is a code owner

@xinyual xinyual Awaiting requested review from xinyual xinyual is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Hailong-am @pyek-bot @dhrubo-os @ylwu-amzn @mingshl @b4sjoo